Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not calculate folder size for parent that also needs proper scan, fixes #3524 #14425

Merged

Conversation

ariselseng
Copy link
Member

As discussed in #3524
This small patch avoids a weird edge case when multiple incomplete folders shares some of their tree and fileid is higher on the lowest folder.

Example:
testfile1.txt and testfile2.txt gets modified and occ files_external:notify registers it by setting size -1 on subfolder1 and subfolder2.
Tree:
subfolder1/testfile1.txt
subfolder1/subfolder2/testfile2.txt

If subfolder2 has higher fileid, only testfile1.txt would be properly scanned when doing a backgroundScan. subfolder1 would just get its old cached size back and not properly scan

Any thoughts? @MorrisJobke @icewind1991
Any chance this could be backported to 15? It seems to be it should be a quite a top priority bug.

@ariselseng
Copy link
Member Author

@icewind1991 @rullzer
I believe this should be backported to 15 too. This actually can cause people to loose data. Even more so than the other notify PR that was merged.

The case I am talking about is that when two folders are marked with size -1 and share some part of its path. Like /Mainfolder and /Mainfolder/Subfolder. Only Subfolder will be scanned correctly and checked for modified files.
/Mainfolder will just get a calculated size back instead of actually checking it for modified files.
The problem is reproducible every time I try.

To reproduce this you can simply do this 3 commands:
In terminal window 1:

$ php occ files_external:notify 1 -v

In a terminal with the SMB share mounted normally with cifs outside of nextcloud:

$ mkdir -p /media/SHARE/Mainfolder/Subfolder
$ touch /media/SHARE/Mainfolder/test.txt
$ touch /media/SHARE/Mainfolder/Subfolder/testsub.txt
$ php occ files:scan --all --unscanned -v 
$ echo "some text" >> /media/SHARE/Mainfolder/test.txt
$ echo "some other text" >> /media/SHARE/Mainfolder/Subfolder/subtest.txt
$ php occ files:scan --all --unscanned -v

In this simple example, only Subfolder along with subtest.txt will have its new size. Mainfolder will have its old size + Subfolders change in size. test.txt will never be scanned.

Let me know if you have any questions :)

@ariselseng
Copy link
Member Author

Alright so I fixed the bug a more proper way I believe. Please review. Though I forgot to sign my revert before I commited the newer fix.

@ariselseng ariselseng force-pushed the fix-multiple-incomplete-folders branch from dc34064 to eb6e8fe Compare March 1, 2019 22:54
@ariselseng ariselseng changed the title Sort by top-most path in getIncomplete(), fixes #3524 Avoid calculating folder size for parent that also needs proper scan, fixes #3524 Mar 1, 2019
@ariselseng
Copy link
Member Author

@icewind1991 Could you check out the new changes if that could be merged?

I am wondering if the changes breaks other stuff.

@ariselseng
Copy link
Member Author

One case that is not handled efficiently is when two folders both have size = -1 and shares parent.
Need to add a check for folders siblings that also needs check and avoid calculating parent if such sibling exists.

@ariselseng
Copy link
Member Author

Update: I pushed more code to handle this scenario:

Masterfolder
Masterfolder/subfolder1/test1.txt
Masterfolder/subfolder2/test2.txt
Masterfolder/[lots of files]

When there is a change in both test1.txt and test2.txt, avoid scanning the whole Masterfolder.
@icewind1991 Any thoughts?

@MorrisJobke MorrisJobke mentioned this pull request Mar 4, 2019
45 tasks
@ariselseng ariselseng added the bug label Mar 4, 2019
@ariselseng ariselseng changed the title Avoid calculating folder size for parent that also needs proper scan, fixes #3524 Do not calculate folder size for parent that also needs proper scan, fixes #3524 Mar 4, 2019
@ariselseng
Copy link
Member Author

This are failing testShallow tests right now. I need to make it only avoid doing correctFolderSize() on parent in a backgroundscan. @icewind1991 Would you accept adding a third parameter to correctFolderSize():

correctFolderSize($path, $data = null, $isBackgroundScan = false)

I have tried that and the tests works then.

@MorrisJobke
Copy link
Member

This are failing testShallow tests right now. I need to make it only avoid doing correctFolderSize() on parent in a backgroundscan. @icewind1991 Would you accept adding a third parameter to correctFolderSize():

correctFolderSize($path, $data = null, $isBackgroundScan = false)

I have tried that and the tests works then.

Makes sense to me 👍 Also the usage is not limiting it, because it's internally used only.

@MorrisJobke
Copy link
Member

Could you then squash all your commits?

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 5, 2019
@ariselseng ariselseng force-pushed the fix-multiple-incomplete-folders branch from 0f294c6 to e5272e3 Compare March 5, 2019 18:24
@ariselseng ariselseng force-pushed the fix-multiple-incomplete-folders branch from e5272e3 to 8605453 Compare March 5, 2019 18:48
@ariselseng ariselseng force-pushed the fix-multiple-incomplete-folders branch from 8605453 to 77b8c47 Compare March 5, 2019 20:21
@ariselseng ariselseng requested a review from kesselb March 5, 2019 20:21
@ariselseng ariselseng added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 5, 2019
Signed-off-by: Ari Selseng <ari@selseng.net>
@ariselseng ariselseng force-pushed the fix-multiple-incomplete-folders branch from 77b8c47 to d16cfb5 Compare March 6, 2019 14:24
@ariselseng
Copy link
Member Author

@MorrisJobke I added a test now to make sure this bug never comes back again. How does it look?

@MorrisJobke MorrisJobke mentioned this pull request Mar 6, 2019
9 tasks
@MorrisJobke
Copy link
Member

@MorrisJobke I added a test now to make sure this bug never comes back again. How does it look?

The tests look really awesome \o/ 👍

@ariselseng
Copy link
Member Author

Alright let's get this reviewed and merged :)

@ariselseng
Copy link
Member Author

@rullzer @icewind1991
Any chance of a review? To be able to understand the problem that this PR solves, you can check out the test written to ScannerTest.php.

@MorrisJobke
Copy link
Member

Status of 16825: failure

DB=sqlite, ENABLE_REDIS=false, PHP=7.3

Show full log
There was 1 failure:

1) TrashbinTest::testExpireOldFiles
Failed asserting that null is identical to 'file2.txt'.

/drone/src/github.com/nextcloud/server/apps/files_trashbin/tests/TrashbinTest.php:186

--

There was 1 risky test:

1) OCA\TwoFactorBackupCodes\Tests\Db\BackupCodeMapperTest::testInsertArgonEncryptedCodes
This test did not perform any assertions

DB=mysqlmb4, ENABLE_REDIS=false, PHP=7.3

Show full log
There was 1 failure:

1) TrashbinTest::testExpireOldFiles
Failed asserting that null is identical to 'file2.txt'.

/drone/src/github.com/nextcloud/server/apps/files_trashbin/tests/TrashbinTest.php:186

--

There was 1 risky test:

1) OCA\TwoFactorBackupCodes\Tests\Db\BackupCodeMapperTest::testInsertArgonEncryptedCodes
This test did not perform any assertions

TESTS=acceptance, TESTS-ACCEPTANCE=app-comments

  • tests/acceptance/features/app-comments.feature:222
Show full log
  Scenario: search a comment                                      # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-comments.feature:222
    Given I am logged in                                          # LoginPageContext::iAmLoggedIn()
    And I open the details view for "welcome.txt"                 # FileListContext::iOpenTheDetailsViewFor()
    And I open the "Comments" tab in the details view             # FilesAppContext::iOpenTheTabInTheDetailsView()
    And I create a new comment with "Hello world" as message      # CommentsAppContext::iCreateANewCommentWithAsMessage()
    And I see a comment with "Hello world" as message             # CommentsAppContext::iSeeACommentWithAsMessage()
      Comment with text "Hello world" in details view in Files app could not be found after 100 seconds (NoSuchElementException)
    When I search for "hello"                                     # SearchContext::iSearchFor()
    Then I see that the search result 1 is "user0Hello world"     # SearchContext::iSeeThatTheSearchResultIs()
    And I see that the search result 1 was found in "welcome.txt" # SearchContext::iSeeThatTheSearchResultWasFoundIn()
sh: 1: kill: No such process

TESTS=acceptance, TESTS-ACCEPTANCE=app-files

  • tests/acceptance/features/app-files.feature:97
Show full log
  Scenario: show favorites for a second time                          # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files.feature:97
    Given I am logged in                                              # LoginPageContext::iAmLoggedIn()
    And I open the "Favorites" section                                # AppNavigationContext::iOpenTheSection()
    And I see that the current section is "Favorites"                 # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    And I open the "All files" section                                # AppNavigationContext::iOpenTheSection()
    And I see that the current section is "All files"                 # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    And I mark "welcome.txt" as favorite                              # FileListContext::iMarkAsFavorite()
    When I open the "Favorites" section                               # AppNavigationContext::iOpenTheSection()
    Then I see that the current section is "Favorites"                # AppNavigationContext::iSeeThatTheCurrentSectionIs()
    Then I see that the file list contains a file named "welcome.txt" # FileListContext::iSeeThatTheFileListContainsAFileNamed()
      Row for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-tags

  • tests/acceptance/features/app-files-tags.feature:42
  • tests/acceptance/features/app-files-tags.feature:70
Show full log
  Scenario: add tags using the dropdown in the details view                                 # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-tags.feature:42
    Given I am logged in as the admin                                                       # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I visit the settings page                                                           # SettingsMenuContext::iVisitTheSettingsPage()
    And I open the "Tag management" section                                                 # AppNavigationContext::iOpenTheSection()
    And I see that the button to select tags is shown                                       # SettingsContext::iSeeThatTheButtonToSelectTagsIsShown()
    And I create the tag "tag1" in the settings                                             # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag2" in the settings                                             # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag3" in the settings                                             # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag4" in the settings                                             # SettingsContext::iCreateTheTagInTheSettings()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag1" # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag2" # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag3" # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag4" # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I log out                                                                           # SettingsMenuContext::iLogOut()
    And I am logged in                                                                      # LoginPageContext::iAmLoggedIn()
    And I open the details view for "welcome.txt"                                           # FileListContext::iOpenTheDetailsViewFor()
    And I open the input field for tags in the details view                                 # FilesAppContext::iOpenTheInputFieldForTagsInTheDetailsView()
    When I check the tag "tag2" in the dropdown for tags in the details view                # FilesAppContext::iCheckTheTagInTheDropdownForTagsInTheDetailsView()
    And I check the tag "tag4" in the dropdown for tags in the details view                 # FilesAppContext::iCheckTheTagInTheDropdownForTagsInTheDetailsView()
    Then I see that the tag "tag2" in the dropdown for tags in the details view is checked  # FilesAppContext::iSeeThatTheTagInTheDropdownForTagsInTheDetailsViewIsChecked()
    And I see that the tag "tag4" in the dropdown for tags in the details view is checked   # FilesAppContext::iSeeThatTheTagInTheDropdownForTagsInTheDetailsViewIsChecked()
    And I see that the input field for tags in the details view contains the tag "tag2"     # FilesAppContext::iSeeThatTheInputFieldForTagsInTheDetailsViewContainsTheTag()
      Failed asserting that false is true.
    And I see that the input field for tags in the details view contains the tag "tag4"     # FilesAppContext::iSeeThatTheInputFieldForTagsInTheDetailsViewContainsTheTag()

  Scenario: remove tags using the dropdown in the details view                                  # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-tags.feature:70
    Given I am logged in as the admin                                                           # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I visit the settings page                                                               # SettingsMenuContext::iVisitTheSettingsPage()
    And I open the "Tag management" section                                                     # AppNavigationContext::iOpenTheSection()
    And I see that the button to select tags is shown                                           # SettingsContext::iSeeThatTheButtonToSelectTagsIsShown()
    And I create the tag "tag1" in the settings                                                 # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag2" in the settings                                                 # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag3" in the settings                                                 # SettingsContext::iCreateTheTagInTheSettings()
    And I create the tag "tag4" in the settings                                                 # SettingsContext::iCreateTheTagInTheSettings()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag1"     # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag2"     # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag3"     # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I see that the dropdown for tags in the settings eventually contains the tag "tag4"     # SettingsContext::iSeeThatTheDropdownForTagsInTheSettingsEventuallyContainsTheTag()
    And I log out                                                                               # SettingsMenuContext::iLogOut()
    And I am logged in                                                                          # LoginPageContext::iAmLoggedIn()
    And I open the details view for "welcome.txt"                                               # FileListContext::iOpenTheDetailsViewFor()
    And I open the input field for tags in the details view                                     # FilesAppContext::iOpenTheInputFieldForTagsInTheDetailsView()
    And I check the tag "tag2" in the dropdown for tags in the details view                     # FilesAppContext::iCheckTheTagInTheDropdownForTagsInTheDetailsView()
    And I check the tag "tag4" in the dropdown for tags in the details view                     # FilesAppContext::iCheckTheTagInTheDropdownForTagsInTheDetailsView()
    And I check the tag "tag3" in the dropdown for tags in the details view                     # FilesAppContext::iCheckTheTagInTheDropdownForTagsInTheDetailsView()
    When I uncheck the tag "tag2" in the dropdown for tags in the details view                  # FilesAppContext::iUncheckTheTagInTheDropdownForTagsInTheDetailsView()
    And I uncheck the tag "tag4" in the dropdown for tags in the details view                   # FilesAppContext::iUncheckTheTagInTheDropdownForTagsInTheDetailsView()
    Then I see that the tag "tag2" in the dropdown for tags in the details view is not checked  # FilesAppContext::iSeeThatTheTagInTheDropdownForTagsInTheDetailsViewIsNotChecked()
    And I see that the tag "tag4" in the dropdown for tags in the details view is not checked   # FilesAppContext::iSeeThatTheTagInTheDropdownForTagsInTheDetailsViewIsNotChecked()
    And I see that the tag "tag3" in the dropdown for tags in the details view is checked       # FilesAppContext::iSeeThatTheTagInTheDropdownForTagsInTheDetailsViewIsChecked()
    And I see that the input field for tags in the details view does not contain the tag "tag2" # FilesAppContext::iSeeThatTheInputFieldForTagsInTheDetailsViewDoesNotContainTheTag()
      Failed asserting that false is true.
    And I see that the input field for tags in the details view does not contain the tag "tag4" # FilesAppContext::iSeeThatTheInputFieldForTagsInTheDetailsViewDoesNotContainTheTag()
    And I see that the input field for tags in the details view contains the tag "tag3"         # FilesAppContext::iSeeThatTheInputFieldForTagsInTheDetailsViewContainsTheTag()

TESTS=acceptance, TESTS-ACCEPTANCE=login

  • tests/acceptance/features/login.feature:9
  • tests/acceptance/features/login.feature:15
  • tests/acceptance/features/login.feature:26
  • tests/acceptance/features/login.feature:38
Show full log
  Scenario: try to log in with valid user and invalid password # /drone/src/github.com/nextcloud/server/tests/acceptance/features/login.feature:9
    Given I visit the Home page                                # FeatureContext::iVisitTheHomePage()
    When I log in with user user0 and password 654321          # LoginPageContext::iLogInWithUserAndPassword()
    Then I see that the current page is the Login page         # LoginPageContext::iSeeThatTheCurrentPageIsTheLoginPage()
    And I see that a wrong password message is shown           # LoginPageContext::iSeeThatAWrongPasswordMessageIsShown()
      Wrong password message in Login page could not be found after 100 seconds (NoSuchElementException)

  Scenario: log in with valid user and invalid password once fixed by admin # /drone/src/github.com/nextcloud/server/tests/acceptance/features/login.feature:15
    Given I act as John                                                     # ActorContext::iActAs()
    And I can not log in with user user0 and password 654231                # LoginPageContext::iCanNotLogInWithUserAndPassword()
      Wrong password message in Login page could not be found after 100 seconds (NoSuchElementException)
    When I act as Jane                                                      # ActorContext::iActAs()
    And I am logged in as the admin                                         # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the User settings                                            # SettingsMenuContext::iOpenTheUserSettings()
    And I set the password for user0 to 654321                              # UsersSettingsContext::iSetTheFieldForUserTo()
    And I act as John                                                       # ActorContext::iActAs()
    And I log in with user user0 and password 654321                        # LoginPageContext::iLogInWithUserAndPassword()
    Then I see that the current page is the Files app                       # FilesAppContext::iSeeThatTheCurrentPageIsTheFilesApp()

  Scenario: try to log in with invalid user                    # /drone/src/github.com/nextcloud/server/tests/acceptance/features/login.feature:26
    Given I visit the Home page                                # FeatureContext::iVisitTheHomePage()
    When I log in with user unknownUser and password 123456acb # LoginPageContext::iLogInWithUserAndPassword()
    Then I see that the current page is the Login page         # LoginPageContext::iSeeThatTheCurrentPageIsTheLoginPage()
    And I see that a wrong password message is shown           # LoginPageContext::iSeeThatAWrongPasswordMessageIsShown()
      Wrong password message in Login page could not be found after 100 seconds (NoSuchElementException)

  Scenario: log in with invalid user once fixed by admin              # /drone/src/github.com/nextcloud/server/tests/acceptance/features/login.feature:38
    Given I act as John                                               # ActorContext::iActAs()
    And I can not log in with user unknownUser and password 123456acb # LoginPageContext::iCanNotLogInWithUserAndPassword()
      Wrong password message in Login page could not be found after 100 seconds (NoSuchElementException)
    When I act as Jane                                                # ActorContext::iActAs()
    And I am logged in as the admin                                   # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the User settings                                      # SettingsMenuContext::iOpenTheUserSettings()
    And I click the New user button                                   # UsersSettingsContext::iClickTheNewUserButton()
    And I see that the new user form is shown                         # UsersSettingsContext::iSeeThatTheNewUserFormIsShown()
    And I create user unknownUser with password 123456acb             # UsersSettingsContext::iCreateUserWithPassword()
    And I see that the list of users contains the user unknownUser    # UsersSettingsContext::iSeeThatTheListOfUsersContainsTheUser()
    And I act as John                                                 # ActorContext::iActAs()
    And I log in with user unknownUser and password 123456acb         # LoginPageContext::iLogInWithUserAndPassword()

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good 👍

@MorrisJobke MorrisJobke merged commit 7723033 into nextcloud:master Mar 8, 2019
@MorrisJobke
Copy link
Member

/backport to stable15

@MorrisJobke
Copy link
Member

/backport to stable14

@backportbot-nextcloud
Copy link

backport to stable14 in #14598

@backportbot-nextcloud
Copy link

backport to stable15 in #14597

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants